Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

make restclient config configurable #1416

Merged
merged 1 commit into from
Jun 22, 2021
Merged

make restclient config configurable #1416

merged 1 commit into from
Jun 22, 2021

Conversation

shiyan2016
Copy link
Contributor

What this PR does / why we need it:
make restclient config configurable

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 1, 2021
@shiyan2016 shiyan2016 force-pushed the feature/add-rest-config branch 2 times, most recently from 433d4f1 to 3683231 Compare June 1, 2021 07:05
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 1, 2021
@shiyan2016 shiyan2016 force-pushed the feature/add-rest-config branch 2 times, most recently from 48a79bd to affb656 Compare June 2, 2021 04:45
@shiyan2016
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@shiyan2016: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@shiyan2016
Copy link
Contributor Author

shiyan2016 commented Jun 2, 2021 •
edited
Loading

@hectorj2f Please help me to review this pr and retrigger the test,thanks.

hectorj2f
hectorj2f reviewed Jun 2, 2021
View reviewed changes
cmd/controller-manager/app/controller-manager.go Outdated
@@ -98,6 +99,8 @@ member clusters and do the necessary reconciliation`,
flags.StringVar(&kubeFedConfig, "kubefed-config", "", "Path to a KubeFedConfig yaml file. Test only.")
flags.StringVar(&kubeconfig, "kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
flags.StringVar(&masterURL, "master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
flags.IntVar(&restConfigQPS, "rest-config-qps", 5, "QPS of rest config.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, are these the default values ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, they are default values.

@hectorj2f
Copy link
Contributor

I re-trigger it but we are always failing the docker image pulling issue. So we need to fix that first :/.

@hectorj2f
Copy link
Contributor

This problem is related to this issue #1359. We need to work on that.

@shiyan2016
Copy link
Contributor Author

This problem is related to this issue #1359. We need to work on that.

ok, thanks.

@hectorj2f
Copy link
Contributor

Please, rebase to get the latest changes.

@shiyan2016
Copy link
Contributor Author

shiyan2016 commented Jun 9, 2021 •
edited
Loading

Please, rebase to get the latest changes.

Done it. Thanks. @hectorj2f

xunpan
xunpan reviewed Jun 17, 2021
View reviewed changes
Copy link
Contributor

@xunpan xunpan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please squash 2 commits into 1.

cmd/controller-manager/app/controller-manager.go Outdated
@@ -98,6 +99,8 @@ member clusters and do the necessary reconciliation`,
flags.StringVar(&kubeFedConfig, "kubefed-config", "", "Path to a KubeFedConfig yaml file. Test only.")
flags.StringVar(&kubeconfig, "kubeconfig", "", "Path to a kubeconfig. Only required if out-of-cluster.")
flags.StringVar(&masterURL, "master", "", "The address of the Kubernetes API server. Overrides any value in kubeconfig. Only required if out-of-cluster.")
flags.IntVar(&restConfigQPS, "rest-config-qps", 5, "QPS of rest config.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • if QPS is a float, it is better to use Float32Var instead.
  • I suggest to use more detailed message for the description. E.g.:
    QPS indicates the maximum QPS to the api-server from this client
    Maximum burst for throttle

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xunpan Done. Please review it again. Thanks.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 18, 2021
@shiyan2016 shiyan2016 force-pushed the feature/add-rest-config branch 2 times, most recently from 496d85a to 50e4778 Compare June 18, 2021 02:58
@xunpan
Copy link
Contributor

xunpan commented Jun 18, 2021

/retest

@xunpan
Copy link
Contributor

xunpan commented Jun 18, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shiyan2016, xunpan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2021
@xunpan
Copy link
Contributor

xunpan commented Jun 18, 2021

@hectorj2f
I don't know how to approve testing for this PR. Please help on this.

The code is fine to me.

@irfanurrehman
Copy link
Contributor

Have trigerred a build.

@shiyan2016
Copy link
Contributor Author 

[Fail] Federated "configmaps" [It] should be created, read, updated and deleted successfully 
/go/src/sigs.k8s.io/kubefed/test/common/crudtester.go:477

[Fail] Federated "replicasets.apps" [It] should be created, read, updated and deleted successfully 
/go/src/sigs.k8s.io/kubefed/test/common/crudtester.go:519

It seems not releated to my pr. @hectorj2f @xunpan @irfanurrehman

@irfanurrehman
Copy link
Contributor

@jimmidyson can this be related to the recent update to github actions? I did rerun this 3 times and each time, one of the first or second executed test failed with timeout (we set single-call-timeout of 1m for e2e test). Curiously all the later tests passed.

@shiyan2016 can you please push a "separate temperory commit" on this PR to update the timeout to a higher value, say 2m. This is only to determine if its just that the tests are executing slower then expected in the new environment, or its something else. Thanks in advance.

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2021
@shiyan2016
Copy link
Contributor Author

single-call-timeout

@irfanurrehman Done for single-call-timeout to 2m. Please retriggle test. Thanks.
.

@irfanurrehman
Copy link
Contributor

/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit f7f7dbb into kubernetes-retired:master Jun 22, 2021
@swiftslee swiftslee mentioned this pull request Oct 19, 2021
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hectorj2f hectorj2f hectorj2f left review comments

@xunpan xunpan xunpan left review comments

@irfanurrehman irfanurrehman Awaiting requested review from irfanurrehman

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@irfanurrehman irfanurrehman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shiyan2016 @k8s-ci-robot @hectorj2f @xunpan @irfanurrehman